Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes for Pandas 2 support #742

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open

Fixes for Pandas 2 support #742

wants to merge 45 commits into from

Conversation

bartbroere
Copy link
Contributor

I have started to work on the Pandas 2 support again. Locally I'm testing with an environment that already uses Pandas 2, and these are the changes that were required to get some tests fixed.

The objective is to not break the Pandas 1.5 support, so these should be safe to merge. Where it seemed useful I added some line comments with the reasoning behind the change.

Some more PRs might follow later this week. Once all tests pass on Pandas 2, we can update the CI to include it in the test matrix.

@bartbroere bartbroere mentioned this pull request Dec 16, 2024
Merged
@pquentin
Copy link
Member

Thank you for this! Will review later this week.

The objective is to not break the Pandas 1.5 support, so these should be safe to merge. Where it seemed useful I added some line comments with the reasoning behind the change.

It's OK to break Pandas 1.5 support in a new release if it brings Pandas 2.0 support.

@bartbroere
Mirror behaviour of installed Pandas version when running value_counts
b6a31bd
@bartbroere
Allow passing arguments to the individual asserters
399200d
@bartbroere
Fix for method _construct_axes_from_arguments no longer existing
c198cc9
@bartbroere
Skip mad metric if it does not exist
b18036d
@bartbroere
Account for pandas 2.0 timestamp default behaviour
470848c
@bartbroere
Deal with empty vs other inferred data types
19ad96d
@bartbroere
Account for default datetime precision change
0ad66a0
@bartbroere
Run Black
388722c
@bartbroere
Solution for differences in inferred_type only
416ec1e
@bartbroere
Fix csv and json issues
73078d3
@bartbroere
Skip two doctests
83df6dd
@bartbroere
Passing a set as indexer is no longer allowed
81b381c
@bartbroere
Don't validate output where it differs between Pandas versions in the…
1146b2e 
… environment
@bartbroere
Update test matrix and packaging metadata
02f1573
@bartbroere
Update version of Python in the docs
d08a29a
@bartbroere
Update Python version in demo notebook
d70e2b9
@bartbroere
Match noxfile
b8b1526
@bartbroere bartbroere changed the title Several fixes for the tests to prepare for Pandas 2 support Fixes for Pandas 2 support Dec 19, 2024
@pquentin
Copy link
Member

pquentin commented Jan 6, 2025

buildkite test this please

@bartbroere
Copy link
Contributor Author

It looks like we encountered the next error in CI/CD, the documentation is still being built using a Python 3.8 install with Pandoc:

ERROR: Package 'eland' requires a different Python: 3.8.10 not in '<3.13,>=3.9'
--
  | nox > Session docs failed.
  | 🚨 Error: The command exited with status 1

Any ideas on how to address it? Should I revert the changes in setup.py or see if we can upgrade the runner that builds the documentation?

@pquentin pquentin mentioned this pull request Jan 14, 2025
Merged
@pquentin
Copy link
Member

We can revert this revert when #746 is merged.

buildkite test this please

@pquentin
Copy link
Member

buildkite test this please

@pquentin
Copy link
Member

buildkite test this please

@bartbroere
Copy link
Contributor Author

The build error seems to be unrelated to the code changes: https://buildkite.com/elastic/eland/builds/588#01948f2a-3f56-4201-91c8-e43a29821fa7/534-565

pquentin
pquentin reviewed Jan 27, 2025
View reviewed changes
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thank you! My comments are mostly about making this easier to maintain going forward. Again, I really appreciate the time you took for this. This will ship in the upcoming 8.18 and 9.0 releases of Eland.

eland/dataframe.py Show resolved Hide resolved
docs/sphinx/examples/demo_notebook.ipynb Outdated Show resolved Hide resolved
eland/etl.py Show resolved Hide resolved
eland/etl.py Outdated Show resolved Hide resolved
eland/field_mappings.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
eland/operations.py Outdated Show resolved Hide resolved
eland/operations.py Outdated Show resolved Hide resolved
tests/notebook/test_demo_notebook.ipynb Outdated Show resolved Hide resolved
tests/notebook/test_demo_notebook.ipynb Outdated Show resolved Hide resolved
bartbroere
bartbroere commented Jan 28, 2025
View reviewed changes
Copy link
Contributor Author

@bartbroere bartbroere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with all the suggestions, so I'll address them one at a time and close the corresponding threads.

eland/etl.py Show resolved Hide resolved
eland/field_mappings.py Outdated Show resolved Hide resolved
eland/operations.py Outdated Show resolved Hide resolved
bartbroere
bartbroere commented Jan 29, 2025
View reviewed changes
tests/common.py Outdated Show resolved Hide resolved
bartbroere
bartbroere commented Jan 29, 2025
View reviewed changes
eland/operations.py Outdated Show resolved Hide resolved
@pquentin
Copy link
Member

buildkite test this please

bartbroere
bartbroere commented Jan 30, 2025
View reviewed changes
eland/dataframe.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pquentin pquentin pquentin left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bartbroere @pquentin